feat: support PGO file #77
Conversation
|
@v1v thank you for working on this, I think this pull based approach should work for PGO in MIS. Just to double check with you that I understand it correctly, to target an individual CPU profile PGO file from apm-managed-service we will need to use a similar configuration for updatecli, see an example for apm-ingest-service. # Config file for `updatecli compose ...`.
# https://www.updatecli.io/docs/core/compose/
policies:
- name: Handle pgo files
policy: ghcr.io/elastic/oblt-updatecli-policies/apm-managed-service/pgo:0.1.0
values:
- .ci/updatecli/values.d/pgo.ymlscm:
enabled: true
owner: elastic
repository: oblt-updatecli-policies
username: obltmachine
branch: main
commitusingapi: true
# use one existing file until the pgo file is in the repository.
pgo_file: default.pgo
pgo_target_path: .
pgo_source_path: pgo/apm-ingest-service/default.pgo |
| automerge: {{ .automerge }} | ||
| labels: | ||
| - dependencies | ||
| description: |- |
There was a problem hiding this comment.
For description we actually need something more fancy, we want to print a small diff summary between old version of the PGO file and the new version that is getting raised in the PR. So it's easy to quickly preview what has changed and sanity check the differences. Right now it's done with the go tool pprof in https://github.com/elastic/apm-managed-service/pull/1365/files#diff-fc41f706e66d7e247dec9daa0e605237c1abcb49144cc0685892d82ab165279bR7, we will need to do something similar here as well.
There was a problem hiding this comment.
We use gh api to collect the PR reference, so the PRs in the MIS projects will contain a reference in the PR description. I was thinking we could use the git commit title, but IIUC, go tool pprof needs to run for each specific MIS project.
Therefore, I need to figure out whether we can use something in the updatecli to run the diff, let me figure out if I can support that.
Do you happen to have two pgo files I can use to run go tool pprof and test if my implementation with the diff works as expected?
There was a problem hiding this comment.
Here few PGO files from the apm-server
go tool pprof -top -base=old.pgo.log new.pgo.log
Correct |
|
@v1v could we move forward with merging these changes? |
I want to run some manual testing using the PGO files you sent me, I plan to work on it today |
|
I've tested the pgo diff and it's not working as I wanted, there are some failures when adding the diff: Even though the content of the diff is found if I don't use it for creating the PR: I guess something is not right in the content to be interpolated... However, if no diff, then PRs are created:
If no strong opinion, I'd remove the pgo diff in this PR, so we can use as is and then figure out the pgo diff in a follow-up (either finding the root issue, or maybe adding a GH workflow in charge of adding a comment with the diff) What do you think? |
|
@v1v I think, it's fine to add PGO diff in a followup. I have no concern against merging this change as-is for now. Thanks for experimenting with this. |
As long as the
default.pgofile exists in https://github.com/elastic/apm-managed-service/ then this shared policy can be used to create the relevant PRs in the MIS projects.Part of https://github.com/elastic/observability-robots/issues/2563
Producer
https://github.com/elastic/apm-managed-service/ needs to create the PGO file in the main branch.
Consumers
They need to create the file
updatecli-compose.yamlthat includesand the relevant GitHub action running this.
For instance, see https://github.com/elastic/apm-server/blob/main/updatecli-compose.yaml and https://github.com/elastic/apm-server/blob/main/.github/workflows/update-compose.yml
Why
This approach has been done for the APM agent specs also:
Follow ups